-
Notifications
You must be signed in to change notification settings - Fork 2
feat: inject NODE_IP env var in all containers #339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Sylvain Rabot <[email protected]>
WalkthroughThe PR introduces NODE_IP environment variable injection into Kubernetes container deployments. A new Changes
Sequence DiagramsequenceDiagram
participant DeploymentMutationChain as Deployment<br/>Mutation Chain
participant withNodeIP as withNodeIP<br/>Mutator
participant Container as InitContainer &<br/>Container
DeploymentMutationChain->>withNodeIP: Apply mutators to Deployment
activate withNodeIP
withNodeIP->>Container: Iterate InitContainers
withNodeIP->>Container: Create NODE_IP env var with<br/>FieldRef to status.hostIP
Container->>Container: Append NODE_IP to Env
withNodeIP->>Container: Iterate regular Containers
Container->>Container: Append NODE_IP to Env
deactivate withNodeIP
withNodeIP-->>DeploymentMutationChain: Return mutated Deployment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add unit and integration tests to verify that NODE_IP environment variable is correctly injected in all containers and init containers using the Kubernetes downward API (status.hostIP). Test coverage includes: - Empty deployments - Deployments with existing environment variables - Deployments with init containers - Deployments with multiple containers Also includes formatting fixes from make fmt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/resources/applications/application.go (1)
340-362: Consider adding deduplication logic for idempotency.The mutator appends NODE_IP to all containers without checking if it already exists. While Kubernetes will use the last value if duplicates exist (making this non-critical), adding a check would improve idempotency:
func (a Application) withNodeIP(ctx core.Context) core.ObjectMutator[*appsv1.Deployment] { return func(deployment *appsv1.Deployment) error { nodeIPEnvVar := corev1.EnvVar{ Name: "NODE_IP", ValueFrom: &corev1.EnvVarSource{ FieldRef: &corev1.ObjectFieldSelector{ FieldPath: "status.hostIP", }, }, } for i, container := range deployment.Spec.Template.Spec.InitContainers { + // Skip if NODE_IP already exists + exists := false + for _, env := range container.Env { + if env.Name == "NODE_IP" { + exists = true + break + } + } + if !exists { container.Env = append(container.Env, nodeIPEnvVar) - deployment.Spec.Template.Spec.InitContainers[i] = container + deployment.Spec.Template.Spec.InitContainers[i] = container + } } // Similar logic for Containers... return nil } }Note: This pattern also applies to
withEELicenceandwithJsonLoggingmutators, suggesting a broader codebase pattern that could be improved in a separate refactoring effort.internal/resources/applications/application_test.go (1)
66-204: Consider adding a test case for duplicate injection scenario.To ensure idempotent behavior, consider adding a test case that verifies the mutator's behavior when NODE_IP already exists:
{ name: "deployment with existing NODE_IP", deployment: &appsv1.Deployment{ Spec: appsv1.DeploymentSpec{ Template: v1.PodTemplateSpec{ Spec: v1.PodSpec{ Containers: []v1.Container{ { Name: "main", Image: "test:latest", Env: []v1.EnvVar{ {Name: "NODE_IP", Value: "existing_value"}, }, }, }, }, }, }, }, },This would help verify whether duplicates are created or if the existing value is preserved/overwritten.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
api/formance.com/v1beta1/benthos_types.go(1 hunks)internal/resources/applications/application.go(2 hunks)internal/resources/applications/application_test.go(1 hunks)internal/resources/orchestrations/deployments.go(1 hunks)internal/resources/payments/deployments.go(1 hunks)internal/tests/application_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/resources/applications/application.go (2)
internal/core/context.go (1)
Context(10-16)internal/core/utils.go (1)
ObjectMutator(70-70)
internal/resources/payments/deployments.go (6)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)api/formance.com/v1beta1/payments_types.go (1)
Payments(45-51)api/formance.com/v1beta1/database_types.go (1)
Database(73-79)internal/resources/registries/registries.go (1)
ImageConfiguration(18-23)internal/resources/applications/liveness.go (1)
ProbeOpts(24-24)
internal/resources/applications/application_test.go (1)
internal/resources/applications/application.go (1)
Application(122-127)
internal/tests/application_test.go (1)
internal/core/env.go (1)
Env(11-16)
🔇 Additional comments (7)
api/formance.com/v1beta1/benthos_types.go (1)
40-40: LGTM! Formatting change only.This is a non-functional whitespace adjustment with no semantic impact.
internal/resources/payments/deployments.go (1)
293-293: LGTM! Formatting change only.The trailing comma removal is a minor style adjustment with no functional impact.
internal/resources/applications/application.go (2)
340-362: Well-implemented mutator following established patterns.The
withNodeIPmutator correctly injects the NODE_IP environment variable using FieldRef tostatus.hostIPfor both init containers and regular containers. The implementation is consistent with similar mutators likewithEELicenceandwithJsonLogging.
385-385: LGTM! Mutator correctly integrated into the chain.The
withNodeIPmutator is appropriately positioned in the mutator chain and will be applied to all deployments.internal/resources/orchestrations/deployments.go (1)
187-187: LGTM! Formatting change only.This is a non-functional alignment adjustment with no semantic impact.
internal/resources/applications/application_test.go (1)
66-204: Comprehensive test coverage for NODE_IP injection.The test thoroughly validates NODE_IP injection across multiple deployment scenarios including init containers, multiple containers, and existing environment variables. The assertions correctly verify the FieldRef configuration.
internal/tests/application_test.go (1)
61-96: Excellent integration test coverage.The integration test effectively validates NODE_IP injection in a real deployment scenario with proper async handling using
Eventually. The test thoroughly checks both init containers and regular containers, verifying the correct FieldRef configuration tostatus.hostIP.
https://github.com/orgs/formancehq/discussions/1960